Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

Sign up for GitHub

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

Jump to bottom

Scripts: Update Node and NPM versions in check-engines script. #19680

Closed
wants to merge 1 commit into from
Closed

Scripts: Update Node and NPM versions in check-engines script. #19680

wants to merge 1 commit into from

Conversation

epiqueras
Copy link
Contributor

No description provided.

@epiqueras epiqueras added the [Package] Scripts /packages/scripts label Jan 15, 2020
@epiqueras epiqueras added this to the Future milestone Jan 15, 2020
@epiqueras epiqueras requested a review from gziolo January 15, 2020 17:56
@epiqueras epiqueras self-assigned this Jan 15, 2020
aduth
aduth reviewed Jan 17, 2020
View reviewed changes
Copy link
Member

@aduth aduth left a comment

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

What's the guideline we're using as a reference here?

Is it based on the Getting Started recommendation of "latest active LTS"?

If so:

  • Does this recommendation necessarily extend to @wordpress/scripts, which is intended to be used as a utility independent from Gutenberg? I think it's a reasonable case to be made that the defaults can adhere to Gutenberg's own guidelines
  • What about NPM version? Is that assumed to be the version shipped with the latest active LTS?

We used to be very explicit about this in the documentation:

  • Current latest active LTS release
  • Latest NPM version

This explicitness was lost as part of the changes of #17004. I reintroduced some in #18923, but apparently missed the NPM version. I think many of the issues we're seeing with frequent changes to package-lock.json (example) can be attributed to the fact we don't really have explicit or enforced guidelines around this.

So, couple things I suggest:

Here:

  • Decide if we anticipate we'd want to reintroduce "latest NPM" as the recommended version, and if so, update the NPM version to reflect (at least closer to) what that is currently (6.13.6).
    • Ideally, this is something we don't have to maintain manually. See also related "Separately" item.
  • Add a CHANGELOG entry, mostly in that I think we should consider whether this be treated as a breaking change, since we'd want to bump the major in the next publish

Separately:

  • Clarify NPM version in "Getting Started" guide
  • Add support to read from project local .nvmrc in this script (if it doesn't already exist)
  • If intending to have this follow Gutenberg recommendations, consider having these values be dynamically inferred (retrieve latest Node LTS version, retrieve latest NPM version). I seem to recall some discussion about this previously, so there could be some existing issue tracking it.

@aduth aduth mentioned this pull request Jan 17, 2020
Merged
@aduth
Copy link
Member

aduth commented Jan 17, 2020

I seem to recall some discussion about this previously, so there could be some existing issue tracking it.

Issue: #14201

@epiqueras
Copy link
Contributor Author

I opened this on a suggestion from @gziolo because it immediately fixes the incompatibility issues that were blocking contributions that added new dependencies with no apparent drawbacks. I think the version numbers are just the most conservative bump that fixes the issue.

I think we should enforce LTS for everything and use nvm to resolve them, but that shouldn't block this PR.

@ntwb
Copy link
Member

ntwb commented Jan 18, 2020

Note:

I've created https://meta.trac.wordpress.org/ticket/4974 to have the w.org "build server" upgraded, merging this PR or #18048 and merging to w.org SVN may cause explosions before this upgrade occurs

@ntwb ntwb mentioned this pull request Jan 18, 2020
Merged
@ntwb
Copy link
Member

ntwb commented Jan 18, 2020

Does this recommendation necessarily extend to @wordpress/scripts, which is intended to be used as a utility independent from Gutenberg? I think it's a reasonable case to be made that the defaults can adhere to Gutenberg's own guidelines

I think this is trickier, I think @wordpress/scripts should support active LTS versions, right now that includes 10.x and 12.x

For instance, a project I'm working on uses @wordpress/scripts, but this project is still for the time being a Node.js 10.x project, I hope to get it to Node.js 12.x soon, but that will still take another month or so.

When a Node.js LTS is dropped, @wordpress/scripts should also get a major version bump (likewise for npm, when npm releases 7.x)

What about NPM version? Is that assumed to be the version shipped with the latest active LTS?

npm does not have LTS versions, just the latest version

If my recollection serves me correctly npm dropped any LTS policies it previously had in npm 5.x

@epiqueras
Copy link
Contributor Author

@ntwb What's the status of this in Core?

gziolo
gziolo reviewed Feb 18, 2020
View reviewed changes
packages/scripts/scripts/check-engines.js Outdated
'--node', '>=10.0.0',
'--npm', '>=6.0.0',
'--node', '>=12',
'--npm', '>=6.1',
Copy link
Member

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

We set the minimum version of npm to 6.9 to be able to use aliases with Prettier.

Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

Updated

@aduth
Copy link
Member

aduth commented Apr 10, 2020

As far as I can tell, the primary issue here was with Prettier compatibility, where the NPM minimum version was separately updated as part of #18048.

There's still a question as to whether this should be aligned closer to Gutenberg's development requirements. This is tracked at #14201.

@aduth aduth closed this Apr 10, 2020
@aduth aduth deleted the update/check-engines-versions branch April 10, 2020 20:17
Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Reviewers

@gziolo gziolo gziolo left review comments

@aduth aduth aduth left review comments

@ajitbohra ajitbohra Awaiting requested review from ajitbohra

@nerrad nerrad Awaiting requested review from nerrad

@ntwb ntwb Awaiting requested review from ntwb

@youknowriad youknowriad Awaiting requested review from youknowriad

Assignees

@epiqueras epiqueras

Labels
[Package] Scripts /packages/scripts
Projects
None yet
Milestone
Future
Development

Successfully merging this pull request may close these issues.
4 participants
@epiqueras @aduth @ntwb @gziolo